There is a bit of compilation that happens on ruleset construction, so, for speed, you might want to eventually construct this once, in global scope, and keep it around.

Ah thanks; makes sense. What's happening behind the scenes there?

This re-creates and re-runs the ruleset for each feature, right? If this is all in a single ruleset we can just run it once, can't we?

The string interpolation for feature has no effect here, just pass it directly.

This is defined in two spots, can we define it once and import it?

Ternary statements aren't syntactically clear, so it's worth avoiding them unless they are clearer to read than the alternative. Same for the return statement below.

I don't understand the comment here.

For training, the trainer add-on imports Fathom as a submodule, and its utilities file is called "utilsForFrontend" rather than "utils".

This is one of the two action items @erikrose was going to work on. I will make it more clear in the meantime.

Storing large conditions like this in a variable makes it easier to read the if statement:

const isElementHidden = (
  style.visibility === 'hidden'
  || style.display === 'none'
  || style.opacity === '0'
  || style.width === '0'
  || style.height === '0'
);
if (elementIsHidden) {
  return false;
}

You mean in the context of our top 5? I don't think we should make that assumption. Can we not just take the absolute value of the delta?

Taking the absolute value of the delta alone won't work, as if the element overlaps with the image element in the x-axis, it would score higher than an element that doesn't. However, I can calculate a deltaX based on whether the element is to the left or right of the image (without overlapping) to cover cases where the element is to the left of the image.

The two asterisks are used for doc comments at the start of a function/class/variable. This is just a regular comment and should use //.

I automatically associate // with single line comments and /* */ with multi-line comments.

A /* */ comment would also be fine. A block comment that starts with two stars (/**) instead of one (/*) is specifically a doc comment and should follow JSDoc conventions, and should only occur either immediately above a variable/class/function/module that it is documenting, or be one of the few exceptions (such as type definitions).

Why is the Y axis used for eligibility but the X axis is used for scoring?

Y-axis
For price, there is little correlation in vertical proximity to the image, as some pages' price is toward the top of the image, and in others, it's toward the middle. On a lot of pages, like the one below, there are lots of priceish candidates that could score higher than the correct price depending on whether I choose the center or top of the image as the reference point.

The title is consistently near the top of the image, so it might benefit from being scored by vertical proximity.

X-axis
For the X-axis, there is a reliable correlation between x-proximity and the correct price (see example page above). I don't use this function to score title as I did not see a correlation there (also titles can be above the image, which breaks this rule).

FWIW: I just tried making the X function a binary check like the Y function, and I got much worse accuracy (80% compared to 94% on price).
I also tried making the Y function a fuzzier measure, scoring it like the X. Unfortunately for price, on some pages it's closer to the top of the image, on others it's closer to the center or center bottom. Accuracy on price went down from 94% to 74%. Interestingly for title, the title elements are consistently closer to the top of the image, and making that change brought accuracy up from 96% to 100%.

Conclusion
In conclusion, I am going to change the binary Y-axis rule to a score-based rule for title, and I am going to leave the binary Y-axis rule as-is for price. I am also going to leave the score-based X-axis rule as-is for price, though I renamed it for clarity, since that rule is only used for price and not other features.

It might be worth leaving a short comment explaining that Y axis scoring has worse results.

In general it's more readable when splitting stuff into multiple lines to not put part of what you're splitting on the first line. Maybe change this to:

if (
  DOMRect.top >= (imageDOMRect.top - TOP_BUFFER)
  && DOMRect.bottom <= imageDOMRect.bottom
) {

Or even better, store the comparison in a variable that describes what it's computing.

I understand the dependency and the limitation that FathomFox can only train based on a single type, but if it's okay to leave this uncommented during a training run, why do we need to comment out the other ones during a training run?

I just checked this, and you're right that we don't need to comment out rules in a ruleset that we're not using to train a particular feature. Will remove those comments.

Ah thanks; makes sense. What's happening behind the scenes there?

For training, the trainer add-on imports Fathom as a submodule, and its utilities file is called "utilsForFrontend" rather than "utils".

This is one of the two action items @erikrose was going to work on. I will make it more clear in the meantime.

After talking with Osmose in slack to clarify this, I will submit a separate commit to this PR to address this issue with more detail there.

Interesting! I didn't know that; seems to check out though.

You have said previously that abbreviations/acronyms should be capitalized, so that's why I have it capitalized. Perhaps it becomes confusing though when there are no other terms in the variable name? e.g. ID versus parentID?

I can rename the method to hasDifferentInnerTextThanChildren, since that's really what's being checked.

Effectively what this does is remove elements who have the same innerText as their child, since it is used in a when clause in a rule further down in this file.

I observed that a lot of <div> and <span> elements are style wrappers on other elements. Rather than keeping all of those "redundant" innerText elements in the game and scoring them (they'd end up with the same or similar score as the innermost element), I'd like to eliminate them right at the beginning. This has two benefits:

1. Performance: the more elements you eliminate from scoring, the less computation is needed to find the highest scoring element.
2. Training: When I tag an element as the "correct" price element, I can only choose one for each feature. I was getting wrong accuracy numbers, since the value of the innerText was correct, but the specific element Fathom was choosing would vary from the innermost element to one of its ancestors.

Taking the absolute value of the delta alone won't work, as if the element overlaps with the image element in the x-axis, it would score higher than an element that doesn't. However, I can calculate a deltaX based on whether the element is to the left or right of the image (without overlapping) to cover cases where the element is to the left of the image.

I automatically associate // with single line comments and /* */ with multi-line comments.

Y-axis
For price, there is little correlation in vertical proximity to the image, as some pages' price is toward the top of the image, and in others, it's toward the middle. On a lot of pages, like the one below, there are lots of priceish candidates that could score higher than the correct price depending on whether I choose the center or top of the image as the reference point.

The title is consistently near the top of the image, so it might benefit from being scored by vertical proximity.

X-axis
For the X-axis, there is a reliable correlation between x-proximity and the correct price (see example page above). I don't use this function to score title as I did not see a correlation there (also titles can be above the image, which breaks this rule).

FWIW: I just tried making the X function a binary check like the Y function, and I got much worse accuracy (80% compared to 94% on price).
I also tried making the Y function a fuzzier measure, scoring it like the X. Unfortunately for price, on some pages it's closer to the top of the image, on others it's closer to the center or center bottom. Accuracy on price went down from 94% to 74%. Interestingly for title, the title elements are consistently closer to the top of the image, and making that change brought accuracy up from 96% to 100%.

Conclusion
In conclusion, I am going to change the binary Y-axis rule to a score-based rule for title, and I am going to leave the binary Y-axis rule as-is for price. I am also going to leave the score-based X-axis rule as-is for price, though I renamed it for clarity, since that rule is only used for price and not other features.

I just checked this, and you're right that we don't need to comment out rules in a ruleset that we're not using to train a particular feature. Will remove those comments.

The order of values returned by Object.values is implementation-specific and shouldn't be relied upon. We can't guarantee that the coefficients are returned in the same order every time.

We could:

• Store an array in the JSON file.
• Have a utility function that takes in the coefficient object and returns the coefficients in the correct order, perhaps as a static method on RulesetMaker.
• Change RulesetFactory to take the coefficient object in as a parameter instead of the list. I'm not sure how we'd modify the trainees file though.

Okay I went for option two.

This is still running the ruleset against the document three separate times, but we only need to run it once and extract all three features, don't we?

This overwrites the value if the feature was image since the if statement is missing an else.

I was unaware you could use this in destructured arrays, huh!

Yeah it took me a while to figure out how I could use destructuring here.

The whole "falling trapezoid" function thing is still very confusing to me. I feel like comments describing what we're doing in context would make things easier:

// If the node is below the fold by more than a viewport's length, return a low score.
if (top >= upperHeightLimit) {
  return ZEROISH * featureCoeff.
}

// If the node is above the fold, return a high score.
if (top <= viewportHeight) {
  return ONEISH * featureCoeff;
}

// Otherwise, scale the score linearly between the fold and a viewport's length below it.
const slope = (ONEISH - ZEROISH) / (viewportHeight - upperHeightLimit);
return (slope * (top - upperHeightLimit) + ZEROISH) * featureCoeff;

Sorry if I'm being picky, I'm just trying to ensure I won't go blank next time I look at this. :P

This doc comment seems unnecessary given that the ruleset has a key and it's clear from the rest of the file that these are rulesets. The ruleset name comment can probably go as well.

Probably also unnecessary to comment this function, the signature of these functions is explained above already.

This comment doesn't really add anything either.

This function doesn't change between the features, so we can define it once above and use it three times here.

This file is also extraction-related, and should be placed inside the extraction folder along with the other files.

Why can't we just remove extraction from the imports here? Then the file doesn't need to be moved.

The fathom-trainees add-on assumes trainees.js is dropped into the ./src folder. With a config.js dependency in the WebExtension, we would need to change some paths around (either the extraction subfolder or the path to config.js for training).

However, this comment made me realize my reason for using config.js was no longer necessary, so having removed it, I can put all the scripts needed for training into the top level ./src folder without this additional step! Thanks.